[WIP] check if Onyx is ready when app is authenticating #5888

marcochavezf · 2021-10-15T15:52:34Z

Problem

There's an issue on Android where the user is logged out randomly. We're not 100% sure about the root cause, but one possibility is the existence of a race condition between the async storage and the networks requests; the Onyx.connect callback to retrieve the stored authToken could be called after the Network.registerParameterEnhancer callbacks (which adds the authToken in the requests) and if the authToken is not ready when a request is made then the user is logged out.

I was able to reproduce this scenario by adding a setTimeout in the Onyx.connect callback (more details in this comment):

Screen.Recording.2021-10-12.at.18.32.38.mov

Proposed workaround and solution

I think the long-term solution should be a restructuring of the handling between the async storage and the network layer in order to wait first for the async storage (Onyx) to be resolved to then set up the network layer.

But since we're only 100% sure if this race condition exists and is the cause of the random logout I propose to add a flag to check if authToken is ready from Onyx and log to the server a message if the race condition happens. This flag also skips adding the default parameters when Onyx is not ready; in that way, we avoid making a request without the authToken (which hasn't been retrieved from Onyx) and thus avoid logging out the user.

If the race condition exists, the message Onyx is not ready in Network.registerParameterEnhancer is logged to the server, so with that can be sure that:

A. Onyx not being ready could be the real issue and will require restructuring the app bootstrapping to wait first for Onyx to be ready before making any network request.

or

B. If the random logout is still happening and the message is not found in the logs we ensure that a delay in Onyx.connect is not the culprit of the random logout.

Fixed Issues

$ #5619

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

fix setTimeout

kidroca

IMO this doesn't solve the root problem we're just skipping adding the default parameters when we can't

If we're making a request that needs an auth token it will just fail, which would trigger re-authentication and so on
We could have a perfectly valid token and this will forfeit it and delay the original request even more

It adds a complication that requires significant explanation, and this just solves the authToken case, while we have other keys like credentials that might be suffering from the same problem

Obtaining (and waiting for) the storage values right before the request is made is the right way to go IMO
At that point you're already in an async operation - the request call. You can start another async operation like reading from storage and then chain the request to it. Reading from storage would either happen instantly (if the key is cached) or will take some time.
This way the code is self-explanatory - we don't have to explain that Onyx or the token is not ready

kidroca · 2021-10-22T12:49:22Z

src/libs/API.js

+        return addDefaultValuesToParameters(command, parameters);
+    }
+
+    LogUtil.hmmm('Onyx is not ready in Network.registerParameterEnhancer ', {command, parameters});


Not returning here means the network call is probably losing it's parameters

App/src/libs/Network.js

Lines 199 to 213 in 886aa24

const finalParameters = _.isFunction(enhanceParameters)

? enhanceParameters(queuedRequest.command, requestData)

: requestData;

// Check to see if the queue has paused again. It's possible that a call to enhanceParameters()

// has paused the queue and if this is the case we must return. We don't retry these requests

// since if a request is made without an authToken we sign out the user.

if (!canMakeRequest(queuedRequest)) {

return;

}

HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)

.then(response => onResponse(queuedRequest, response))

.catch(error => onError(queuedRequest, error));

});

If the idea is to acknowledge and still let the request to go through without the default values, we should at least return the parameters here

marcochavezf · 2021-10-25T22:47:42Z

IMO this doesn't solve the root problem we're just skipping adding the default parameters when we can't

If we're making a request that needs an auth token it will just fail, which would trigger re-authentication and so on We could have a perfectly valid token and this will forfeit it and delay the original request even more

Thanks for the review @kidroca, you're right. I'm going to close this PR and reopen a new one with another solution.

while we have other keys like credentials that might be suffering from the same problem

About the credentials, I'm not really sure if that cause the logout issue, because I've been testing without the credentials data and the app keeps working fine, for example here I even commented out the code to retrieve the credentials and the user is still logged in:

kidroca · 2021-10-26T05:31:38Z

About the credentials, I'm not really sure if that cause the logout issue, because I've been testing without the credentials data and the app keeps working fine

They will only matter when the user needs to reauthenticate. On app launch If the token expired but credentials haven’t been read yet
I think there might be a small chance of that happening

marcochavezf force-pushed the marco-checkIfOnyxIsReady branch 2 times, most recently from 3fbc337 to 54cb877 Compare October 15, 2021 15:57

check if Onyx is ready when app is authenticating

ab7103b

fix setTimeout

marcochavezf force-pushed the marco-checkIfOnyxIsReady branch from 54cb877 to ab7103b Compare October 21, 2021 19:46

remove timeout and add comments

ef9324b

kidroca reviewed Oct 22, 2021

View reviewed changes

marcochavezf closed this Oct 25, 2021

lakchote mentioned this pull request Mar 21, 2025

Deploy Checklist: New Expensify 2025-03-20 #58801

Closed

72 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] check if Onyx is ready when app is authenticating #5888

[WIP] check if Onyx is ready when app is authenticating #5888

marcochavezf commented Oct 15, 2021 •

edited

Loading

kidroca left a comment

kidroca Oct 22, 2021

marcochavezf commented Oct 25, 2021 •

edited

Loading

kidroca commented Oct 26, 2021

	const finalParameters = _.isFunction(enhanceParameters)
	? enhanceParameters(queuedRequest.command, requestData)
	: requestData;

	// Check to see if the queue has paused again. It's possible that a call to enhanceParameters()
	// has paused the queue and if this is the case we must return. We don't retry these requests
	// since if a request is made without an authToken we sign out the user.
	if (!canMakeRequest(queuedRequest)) {
	return;
	}

	HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)
	.then(response => onResponse(queuedRequest, response))
	.catch(error => onError(queuedRequest, error));
	});

[WIP] check if Onyx is ready when app is authenticating #5888

[WIP] check if Onyx is ready when app is authenticating #5888

Conversation

marcochavezf commented Oct 15, 2021 • edited Loading

Problem

Proposed workaround and solution

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

kidroca left a comment

Choose a reason for hiding this comment

kidroca Oct 22, 2021

Choose a reason for hiding this comment

marcochavezf commented Oct 25, 2021 • edited Loading

kidroca commented Oct 26, 2021

marcochavezf commented Oct 15, 2021 •

edited

Loading

marcochavezf commented Oct 25, 2021 •

edited

Loading